feat: add host-validated extension telemetry gRPC service#9174
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
4c63574 to
d1f3de6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
vhvb1989
left a comment
vhvb1989
left a comment
There was a problem hiding this comment.
Design concern — please review before accepting as proposed.
My main concern with this PR is that it overloads the result Artifact.Metadata bag to carry telemetry-only data (azure.ai.agents.deploymentMode) alongside genuine payload information (location, kind, sha256), and then teaches azd core how to fish that value back out based on hardcoded knowledge of a single extension (if extension.Id == "azure.ai.agents" + the extension's private metadata key). This inverts azd's normal dependency direction — core is supposed to stay extension-agnostic, with extensions depending on the SDK, not the other way around. The result is a stringly-typed, cross-module side-channel (the magic strings are duplicated in core and in the extension with no compiler enforcement) that doesn't generalize: any future hosted-agent-style extension gets nothing until someone edits a core allowlist. It seems like a lot of core plumbing for what is ultimately telemetry generation.
I think the cleaner approach is a generic capability — e.g. an SDK/gRPC method that lets an extension set a telemetry attribute directly — so the extension emits agent.deploy.mode itself, Artifact stays purely about payload, and core stays extension-agnostic with no hardcoded extension name. Same telemetry outcome, none of the inverted coupling. Can we discuss whether that direction is preferable before merging this as-is?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
a2bc94c to
5644075
Compare
5644075 to
38fd61a
Compare
huimiu
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- cli/azd/pkg/azdext/telemetry.pb.go: Generated file
- cli/azd/pkg/azdext/telemetry_grpc.pb.go: Generated file
Comments suppressed due to low confidence (2)
docs/specs/metrics-audit/telemetry-schema.md:91
- This field is emit-capable once this host ships, even if the first-party producer follows later. Repository telemetry guidance requires new fields to be reflected in
docs/reference/telemetry-data.mdand new cross-cutting emission paths infeature-telemetry-matrix.md(cli/azd/AGENTS.md:246-281); currently neither mentions this field/service. Add those entries now so the public reference and audit inventory stay aligned with the host behavior. [azd-code-reviewer]
| Agent deployment mode | `agent.deploy.mode` | SystemMetadata | FeatureInsight | `string[]`; per-command set of `code`/`container`/`byo_image` contributed by an authenticated extension through the telemetry service; fixed enum, not hashed; App Insights stores JSON text |
cli/azd/internal/cmd/from_package_test.go:20
- This test only verifies that two constants are equal; it never exercises either changed
--from-packageartifact construction path. Both metadata assignments inservice_graph.goandpublish.gocould be removed while this still passes. Add behavior tests for both paths that inspect the resulting package artifact and assertazd.fromPackage == "true". [azd-code-reviewer]
func TestFromPackageMetadataConstant(t *testing.T) {
t.Parallel()
require.Equal(t, "azd.fromPackage", project.MetadataKeyFromPackage)
require.Equal(t, azdext.ArtifactMetadataKeyFromPackage, project.MetadataKeyFromPackage)
| // service, which routes them to the current scope. Closing the scope below | ||
| // attaches those values to this command span only, so they never leak onto | ||
| // synthetic child spans or sibling commands. | ||
| usageScope := tracing.BeginCommandUsageScope(eventName) |
This comment was marked as off-topic.
This comment was marked as off-topic.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
The generic host-validated telemetry service resolves the coupling concern from my earlier note. Core no longer hardcodes the agents extension id or reads its private metadata key: eligibility now runs through the signed capability check plus the host-owned key/value allowlist, and the from-package marker is a plain provenance boolean with the constant aliased from the SDK (MetadataKeyFromPackage = azdext.ArtifactMetadataKeyFromPackage) so core and extensions can't drift. That's the right direction, and it keeps Artifact.Metadata about payload rather than telemetry.
One thing to settle before merge, and it's your own open item rather than a new one: the privacy review of the new extension-to-host telemetry boundary is still unchecked. The allowlist keeps the strict model intact, but the choice to keep it strict versus loosening to namespaced keys should get a sign-off from the telemetry/privacy owner, since it changes who can put values into azd telemetry. Worth locking that down instead of carrying it as an open question into merge.
The test-coverage and docs gaps the automated reviewer flagged (publish not classified in TestCommandTelemetryCoverage, from_package_test only asserting constant equality) are worth a quick pass. Deferring the emitted-data docs to the producer PR makes sense given nothing emits the field yet.
Fixes #9255. Part of #9230 (the producer lands in a follow-up PR).
Why
We want to know which path a hosted agent deploy actually took: source code (
code), a container azd built (container), or an existing image the user brought (byo_image). The manifest language alone can't tell these apart, so adoption and troubleshooting telemetry is ambiguous today.An earlier version of this PR taught azd core to read that value out of deploy artifact metadata and hardcoded the
azure.ai.agentsextension. That was rejected: it pointed the dependency the wrong way (core knowing about one specific extension), shipped telemetry-only data through the deploy artifact contract, and would force a core change for every future extension.What this PR does
This PR replaces that with a generic, host-controlled way for any authenticated azd extension to contribute a telemetry value, and registers the first field for it. It is core-only: the plumbing and the field are in, but nothing emits a value yet. The agents extension starts producing
agent.deploy.modein a follow-up PR, after this ships in a core release.TelemetryServicewith a singleAddCommandUsageAttribute(key, value)call. An extension calls it; azd core validates the key/value against a host-owned allowlist and attaches the value to the current command's telemetry event (cmd.deployorcmd.up) as a de-duplicated string slice.agent.deploy.modeasSystemMetadata/FeatureInsightwith the fixed valuescode,container,byo_image.service-target-providercapability may report, and only for allowed keys/values. Invalid input is rejected and never echoed into errors or logs.Unimplementedand the extension ignores it.azd.fromPackageartifact provenance marker (payload provenance, not telemetry) so a future producer can tell a user-supplied--from-packagepayload from one azd built.Why this approach
Core stays extension-agnostic: it owns the telemetry schema (key, allowed values, classification, purpose, eligible commands) and validates everything an extension sends. Extensions can't invent telemetry or push arbitrary data, which keeps azd's existing per-field privacy governance intact across the extension boundary. Values are collected per command and flushed onto the correct command span, so they can't leak onto synthetic package/provision spans or later commands.
Open question — the allowlist
Core keeps an allowlist: every key/value an extension can send is pre-registered in core with its classification and eligible commands.
Follow-up (not in this PR)
Tracked in #9230:
agent.deploy.modeat the earliest authoritative decision, on a released core SDK.docs/reference/telemetry-data.md,feature-telemetry-matrix.md) once the field actually emits.Testing
go build, targeted unit and integration tests,gofmt,go vet,golangci-lint(0 issues),go fix -diff(clean), and cspell all pass. Race checks run in CI.Telemetry Change Checklist
fields/fields.gowith correct classification (SystemMetadata) and purpose (FeatureInsight)docs/specs/metrics-audit/telemetry-schema.mdcode,container,byo_image)